Skip to content

feat(natsstore): serve all tenants from a single muxed K/V bucket - #4

Merged
omer9564 merged 7 commits into
mainfrom
omer/single-muxed-kv
Jun 4, 2026
Merged

feat(natsstore): serve all tenants from a single muxed K/V bucket#4
omer9564 merged 7 commits into
mainfrom
omer/single-muxed-kv

Conversation

@omer9564

@omer9564 omer9564 commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

What & why

Replaces the bucket-per-tenant model with a single muxed bucket (config.bucket), keyed "<tenant>.<key...>". At ~40k tenants, one KV bucket per tenant means one JetStream RAFT group per tenant (~40k HA assets, >10GiB RAM, ~20× over the ~2k-HA-assets/server budget). Collapsing to one bucket removes that fixed per-RAFT-group cost.

The Rego interface is unchanged — data still lands at data.nats.kv.<tenant>.<...>, and nats.kv.watch_bucket / nats.kv.get_data keep their names and arity. The only conceptual shift: argument 1 is now the tenant token (the leading key segment in the one bucket) instead of a bucket name. Policies don't change.

No backward compatibility retained (intentional, per request). bucket is now required; old per-bucket configs won't validate.

Core changes

  • config: add required bucket; rename root_bucketroot_tenant (a tenant whose subtree mounts at the OPA data root).
  • NATSKeyToOPAPath: derive the tenant from the key's first token (drop the out-of-band bucket arg); root mode strips the tenant token.
  • nats_client: open the single bucket once (cached); add tenantKeys(), a prefix-filtered lister so loads are O(tenant), never O(whole bucket) — the key correctness/perf change.
  • watcher: watch "<tenant>.>" (single filter → scopeable consumer) instead of ">"; identity is the tenant.
  • builtins: loadTenantAsGJSON reads only the tenant slice and strips the "<tenant>." prefix via the new pure buildTenantJSON helper.

Tests (TDD for the pure logic)

  • Reshaped path-mapping / config / JSON-builder unit tests (watched each fail first, then implemented).
  • New muxed_integration_test.go (real NATS): tenantKeys isolation, data placement at data.nats.kv.<tenant>, t2 never leaking into a t1 load, root-tenant stripping.
  • Migrated the example (one DATA bucket, <tenant>.* keys) + both config files + README; account perms scoped to $KV.DATA.>.

Verification

  • pkg/natsstore suite: ok (incl. the new real-NATS integration test).
  • cmd/opa-nats docker-compose integration test: PASS (54s) — real OPA + NATS, bucket_watched flips false→true, get_data(bucket,"members") returns the members map, data.nats.kv[bucket_id] returns the full tenant tree. Same interface, same placement.
  • go build, go vet, gofmt, golangci-lint (0 issues), all pre-commit hooks pass.

Operational note

At the cloud, each watched tenant is one ordered consumer on KV_DATA, capped by max_bucket_watchers (LRU). WatchFiltered could batch tenants later if fan-in becomes a concern.

🤖 Generated with Claude Code

Replace the bucket-per-tenant model with one muxed bucket (config.bucket),
keyed "<tenant>.<key...>". The Rego interface is unchanged: data still lands at
data.nats.kv.<tenant>.<...> and the builtins (nats.kv.watch_bucket /
nats.kv.get_data) keep their names and arity — their first argument is now the
tenant token (the leading key segment) instead of a bucket name.

Core changes:
- config: add required `bucket`; rename `root_bucket` -> `root_tenant`
  (a tenant whose subtree mounts at the OPA data root).
- NATSKeyToOPAPath: derive the tenant from the key's first token (drop the
  out-of-band bucket arg); root mode strips the tenant token.
- nats_client: open the single bucket once (cached); add tenantKeys(), a
  prefix-filtered lister so loads are O(tenant), never O(whole bucket).
- watcher: watch "<tenant>.>" (single filter -> scopeable consumer) instead of
  ">"; identity is the tenant.
- builtins: loadTenantAsGJSON reads only the tenant slice and strips the
  "<tenant>." prefix via the new pure buildTenantJSON helper.

Tests (TDD for the pure logic):
- reshaped path-mapping/config/JSON-builder unit tests;
- new muxed_integration_test.go (real NATS): tenantKeys isolation, data
  placement, root-tenant stripping;
- migrated the example (one DATA bucket, <tenant>.* keys) + configs; the
  docker-compose integration test passes end to end.

No backward compatibility retained (intentional).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@omer9564
omer9564 requested a review from zeevmoney June 4, 2026 07:25
…le bucket to POLICY_DATA

The muxed bucket name is required configuration for the reader — there must be no
implicit default that could silently point at the wrong bucket. Remove the
"DATA" default from DefaultConfig (Validate still enforces "bucket is required")
and set the example/deployment bucket to POLICY_DATA to match the data-generator
producer (NATS_KV_BUCKET=POLICY_DATA).

- config: DefaultConfig no longer sets Bucket (mandatory, explicit-only).
- example configs + docker-compose seeding + account perms: DATA -> POLICY_DATA.
- README: document `bucket` as required; fix the stale root_bucket -> root_tenant row.
- tests: set Bucket explicitly where DefaultConfig is validated; assert no default.

Verified: pkg suite + cmd docker-compose integration green; golangci-lint 0 issues.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@zeevmoney zeevmoney left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments

Comment thread pkg/natsstore/nats_client.go Outdated
Comment thread pkg/natsstore/plugin.go Outdated
Comment thread pkg/natsstore/plugin.go
Comment thread pkg/natsstore/data_transformer.go Outdated
Comment thread pkg/natsstore/nats_client.go
Comment thread examples/opa-nats/rego/test.rego Outdated
Comment thread README.md
Comment thread pkg/natsstore/muxed_integration_test.go
Comment thread pkg/natsstore/plugin_test.go
Comment thread examples/opa-nats/config.yaml
omer9564 and others added 4 commits June 4, 2026 14:46
- tenantKeys now takes a context.Context, binds the watch with nats.Context(ctx),
  selects on ctx.Done(), and treats a watch channel that closes BEFORE the nil
  initial-values marker as an error instead of silently returning a truncated key
  set (partial tenant data can flip an authz decision). ctx is threaded through
  loadTenantAsGJSON / LoadBucketDataBulk / the builtins.
- buildTenantJSON: a failed Get now returns nil and is SKIPPED (not written as an
  explicit null); the sjson error is handled; non-JSON stored values fall back to
  a JSON string, mirroring loadSingleKey so the read and bulk-load paths agree.
- NATSClient drops the cached bucket handle on reconnect so it is re-opened lazily.
- validateTenant rejects tenant ids that aren't a single safe NATS subject token
  (empty / '.' / '*' / '>' / whitespace) at tenantKeys and the watcher Start, so
  the subject-token watch and the string-prefix read paths cannot diverge.
- Removed the dead DataTransformer.rootBucket field and the now-unused config
  param from NewDataTransformer.
- Gated muxed_integration_test.go behind //go:build integration; switched the
  connect-dependent plugin tests to require.* so they fail cleanly instead of
  nil-deref-panicking when NATS is unreachable.

Addresses review comments:
- #4 (comment) (@zeevmoney)
- #4 (comment) (@zeevmoney)
- #4 (comment) (@zeevmoney)
- #4 (comment) (@zeevmoney)
- #4 (comment) (@zeevmoney)
- #4 (comment) (@zeevmoney)
- #4 (comment) (@zeevmoney)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…bucket sync

- example rego: clarify that non-root tenants land at data.nats.kv.<tenant>.<...>
  while the configured root_tenant mounts at the data root with its token stripped.
- README account perms: note that $JS.API.> is account-wide JetStream access and
  that the bucket name must stay in sync with the $KV.<bucket>.> subjects.

Addresses review comments:
- #4 (comment) (@zeevmoney)
- #4 (comment) (@zeevmoney)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The documented build is `go build -o opa ./cmd/opa-nats`, producing `opa`, which
wasn't ignored; add it (and keep /opa-nats for the bare build). Drop the leading
blank line.

Addresses review comments:
- #4 (comment) (@zeevmoney)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… NATS

The examples stack mounted config.yaml (server_url=localhost:4222) into the
in-container opa service, so localhost resolved to the OPA container and the
plugin couldn't reach the `nats` service. Mount config-compose.yaml
(nats://nats:4222) instead and drop the unused NATS_URL env.

Addresses review comments:
- #4 (comment) (@zeevmoney)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@omer9564
omer9564 requested a review from zeevmoney June 4, 2026 12:10

@zeevmoney zeevmoney left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated Review - the PR looks clean, found nothing.

@zeevmoney

Copy link
Copy Markdown

Code review — informational findings (not inline)

These are from a re-review of omer/single-muxed-kv @ 67c901e. They live on lines this PR doesn't change, so they can't be posted as inline review comments — but two of them are directly exercised by this PR's root_tenant path. None duplicate the (now-resolved) prior review threads. The one in-diff finding was posted as an inline review comment separately.

# Severity Location Description
1 MEDIUM pkg/natsstore/bucket_watcher_manager.go:123 (cleanOPAStore) Always removes nats/kv/<tenant>, ignoring isRoot. A root watcher writes to stripped data-root paths, so on root-watcher Stop/Reconfigure the root data is left resident and the RemoveOp targets a path that was never written. The example config sets root_tenant: "permit-groups-default", so this path is live.
2 MEDIUM pkg/natsstore/builtin_functions.go:89 + bucket_watcher_manager.go:79 Root tenant is bulk-loaded twice: EnsureBucketLoadedLoadBucketDataBulk, then CreateRootWatcherStartLoadBucketDataBulk again. Doubles the per-tenant read the O(tenant) design is meant to minimize.
3 LOW pkg/natsstore/bucket_watcher_manager.go:111-121 (cleanOPAStore defer) The defer runs Abort then unconditionally Commit on the same txn (inner err := shadows the named return). This is not a panic — the inmem store's underlying() rejects the now-stale txn, so the abort is correct and only a spurious "Failed to commit clean transaction: stale transaction" error is logged. Worth tidying so the abort path returns early.
4 LOW pkg/natsstore/data_transformer.go:327 (InjectDataToOPAStore) BasePaths: []string{bucketName} is the tenant token, while writes go to nats/kv/<tenant>/… (or stripped root). Inert for the inmem store (BasePaths is ignored for write txns) but misleading; will mislead/misbehave if a backend ever honors it.
5 LOW pkg/natsstore/plugin.go:117 logger.Warn("Warning: Cache miss … .\n", …) — embedded \n and a redundant Warning: prefix; violates the project "structured logging, no Printf-style" rule.
6 LOW pkg/natsstore/bucket_watcher_manager.go:32 gw.cancel is stored but never called; in-flight tenantKeys/LoadBucketDataBulk bound to gw.ctx can't be cancelled on Stop (the watch loop still exits via stopReq, so this is latent rather than a leak today).
7 MEDIUM pkg/natsstore/*_test.go (coverage) No test for: t1 vs t10 prefix-confusion isolation, cleanOPAStore (either path), root-tenant watch-update writes, or ErrBucketNotFound handling. Real-NATS isolation is only asserted behind the integration build tag, so default go test ./... proves isolation only via the pure buildTenantJSON unit test.

All pre-existing except where noted; raised here for visibility, non-blocking. Happy to open a follow-up issue or push fixes for #1 (root cleanOPAStore) and #2 (duplicate root load) if useful.

@zeevmoney zeevmoney left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved, see one comment and note this one:

#4 (comment)

See if it's safe to merge or defer it to a future issue.

Comment thread pkg/natsstore/plugin.go Outdated
… instead of failing open

watch_bucket special-cased nats.ErrBucketNotFound and returned false. With the
single muxed bucket an absent tenant yields zero keys (handled separately by
loadTenantAsGJSON returning a Null result, no error), so ErrBucketNotFound here
can only mean the configured bucket is missing — a deployment misconfiguration.
Returning false silently evaluates policy against missing data; surface the
error instead, consistent with get_data. Removed the now-unused errors/nats
imports.

Addresses review comments:
- #4 (comment) (@zeevmoney)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@omer9564
omer9564 merged commit ff5b68e into main Jun 4, 2026
4 checks passed
@omer9564
omer9564 deleted the omer/single-muxed-kv branch June 4, 2026 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants